Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Closes #234: No immediate onMessageComplete cb for upgrade w/body (D1364... #235

Closed

Conversation

KjellSchubert
Copy link
Contributor

...677 + D1380182 orig author afrind@fb.com)

@indutny
Copy link
Member

indutny commented Apr 21, 2015

Very interesting! May I ask you to paste a quote from the RFC that allows POST+Upgrade+body? I just want to be sure that we don't introduce any non-standard behavior by this ;)

@KjellSchubert
Copy link
Contributor Author

May I ask you to paste a quote from the RFC that allows POST+Upgrade+body?

I wasn't sure if you were kidding, in any case I checked https://tools.ietf.org/html/rfc7230#section-6.7 & https://tools.ietf.org/html/rfc7231#section-6.2.2 : these talk about upgrade headers but I see no wording that limits this upgrade header to certain kinds of requests: like GET only but not POST, or requests with empty body but not non-empty body. In fact the word 'body' isnt mentioned around the Upgrade headers RFC section. One RFC quote "For example, if the Upgrade header field is received in a GET request and the server decides to switch protocols ..." - this implies to me that the Upgrade header can be contained in other requests but GETs, so why not POSTs with body for example? But in any case, this is still natural language, no one can prove or disprove anything with that :)

You honestly think Upgrade header with HTTP POST (or other HTTP verb + body) could be illegal? If so please quote an RFC that disallows that ;) This would be a weird un-orthogonal protocol limitation imo.

@indutny
Copy link
Member

indutny commented Apr 21, 2015

@KjellSchubert thank you so much! ( I wasn't kidding, btw :) )

It seems that RFC allows it indeed.

LGTM then. @mscdex: may I ask you to take a look at this too? You seem to have lots of insights into HTTP spec.

@mscdex
Copy link
Contributor

mscdex commented Apr 21, 2015

@indutny As far as I can tell from RFC 7230/7231, it appears to be valid. Although I suspect it's about as common as sending a GET request with a body :-)

indutny pushed a commit that referenced this pull request Apr 23, 2015
Invoke message_complete cb for upgrade with body.

(D1364677 + D1380182 orig author afrind@fb.com)

Fix: #234
PR-URL: #235
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Apr 23, 2015

Landed in dff604d, thank you!

@indutny indutny closed this Apr 23, 2015
@samoconnor
Copy link

Hi @mscdex @KjellSchubert @indutny,
I'm a bit late to the party here, but I'd like your opinions on the CONNECT_WITH_BODY_REQUEST test case. The input has Content-Length: 10 and a body "blarfcicle", but the test expects .body= "" !

The RFC says:

The presence of a message body in a request is signaled by a
Content-Length or Transfer-Encoding header field. Request message
framing is independent of method semantics, even if the method does
not define any use for a message body.

i.e. A request that says Content-Length: 10 has a 10-byte body irrespective of the method being CONNECT.

The CONNECT scemantics are defined here: RFC 7231, 4.3.6. It talks about the client ignoring *responses( with Content-Length in, however, that is not relevant to this test request test case.

A server MUST NOT send any Transfer-Encoding or Content-Length header
fields in a 2xx (Successful) response to CONNECT. A client MUST
ignore any Content-Length or Transfer-Encoding header fields received
in a successful response to CONNECT.

RFC 7231, 4.3.6 goes on to say that a payload on a CONNECT request has no defined meaning, but it does not disallow it:

A payload within a CONNECT request message has no defined semantics;
sending a payload body on a CONNECT request might cause some existing
implementations to reject the request.

So, this line may be wrong: https://github.com/nodejs/http-parser/pull/235/files#diff-5c62f371bf37583234d2462ad49ce33dR1837

(Background: @quinnj has ported the node http parser to Julia, which means we've inherited the test cases, and I've been testing it and reviewing it vs the RFC).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants